Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle granular requests configuration #1234

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Handle granular requests configuration #1234

merged 4 commits into from
Dec 5, 2023

Conversation

snutij
Copy link
Contributor

@snutij snutij commented Dec 3, 2023

Motivation

First iteration for: #834, I will follow up with client implementation after this one get validation.

Allow user to configure inlayHints and codeLens requests, and disabling unwanted features.

Implementation

Handle in the server a hash of feature configuration:

  • we set a default configuration so we could merge this without client implementation
  • merge client configurations with default ones
  • early return in requests in case of disabled features

Question: should I disable features by default as proposed here? To my very personal opinion, this can bring confusion with supported features documentation.

Automated Tests

Tests added:

  • in executor to ensure default and provided configurations are well populated
  • in both request to ensure there are no responses in case of disabled feature

Manual Tests

By switching default configuration to false in lib/ruby_lsp/store.rb, then restarting, no feature should be display to user.

Screenshots

inlayHints with true / false default configuration:

image
image

codelens with true / false default configuration:

image
image

@snutij snutij requested a review from a team as a code owner December 3, 2023 16:47
@snutij snutij requested review from KaanOzkan and vinistock December 3, 2023 16:47
@vinistock
Copy link
Member

This is amazing. Thank you for the contribution, Justin!

Question: should I disable features by default as proposed #834 (comment)? To my very personal opinion, this can bring confusion with supported features documentation.

You raise a really good point. We discussed this with the team and I think we can do the following:

  • Let's default all inlay hints to disabled
  • Let's keep code lens as enabled by default
  • Let's update both the documentation for code lens and inlay hints to mention these configurations
  • Finally, let's add an enableAll entry to both configuration hashes. That way, someone interested in having the hints can simply turn everything on instead of needing to add every single one

In terms of code style, it might be worth creating an object in utils.rb to encapsulate the configuration logic. Something along these lines

class RequestConfig
  def initialize(hash)
    @hash = hash
  end

  def enabled?(feature)
    @hash[:enableAll] || @hash[feature]
  end
end

@vinistock vinistock mentioned this pull request Dec 5, 2023
Closed
@snutij snutij force-pushed the handle-granular-request-configuration branch from 5fd37d0 to fbdf5c7 Compare December 5, 2023 15:56
vinistock
vinistock approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I think we're ready to merge, I just wanted to check if we need to return the configured features back to the client. If not, let's remove that line and merge

lib/ruby_lsp/executor.rb Outdated Show resolved Hide resolved
@vinistock
Copy link
Member

🚀

@vinistock vinistock merged commit 005de90 into Shopify:main Dec 5, 2023
13 of 16 checks passed
@snutij snutij mentioned this pull request Dec 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinistock vinistock vinistock approved these changes

@KaanOzkan KaanOzkan Awaiting requested review from KaanOzkan KaanOzkan was automatically assigned from Shopify/ruby-dev-exp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@snutij @vinistock